Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove duplicate dropdown custom renderer #312

Merged
merged 7 commits into from
Jul 24, 2024

Conversation

dcshzj
Copy link
Contributor

@dcshzj dcshzj commented Jul 15, 2024

Problem

A duplicate dropdown renderer appears due to the anyOf renderer and the dropdown custom renderer. This was due to the switch of the schema from being hand-written to TypeBox.

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible

Bug Fixes:

  • Ensure that the dropdown custom renderer does not appear when there is only a single option available.
  • Make the anyOf custom renderer take in the constant value from its children instead of the default label that is auto-generated (which gives something like anyOf-0, anyOf-1, etc).
  • Also prevent the dropdown from selecting the first available option so the user knows what option they are selecting.

Before & After Screenshots

BEFORE:

Screenshot 2024-07-15 at 11 02 44

AFTER:

Screenshot 2024-07-15 at 11 03 11

@dcshzj dcshzj requested a review from a team as a code owner July 15, 2024 10:32
Copy link

vercel bot commented Jul 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
isomer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 24, 2024 4:33am

value: anyOfRenderInfo.label,
}))
const options = anyOfRenderInfos.map((anyOfRenderInfo) => {
const option = String(anyOfRenderInfo.schema.const || anyOfRenderInfo.label)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why choose schema.const over label?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const resembles the actual value that the user will select in the case of a string anyOf, so it would be more appropriate to display that to the user. The label is only used for non-string anyOf (which will not have a const).

Comment on lines 22 to 26
useEffect(() => {
handleChange(path, schema.const)
}, [handleChange, path, schema])

return null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels abit sus - what it feels like we're trying to do here is to write through to our underlying data structure (our data) but not render anything.

i think the pattern that we might want to aim for here might be to use a middleware + reducer so we fire the event only on update and only to mutate data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will likely remove this, I realise we won't have a case where the user will immediately enter with this FormBuilder without the type being provided, so we are guaranteed to have this set.

Actually yeah removed in 492aa3f.

Comment on lines 29 to 37
const [dropdownValue, setDropdownValue] = useState(data || "")

if (!options || (options.length === 1 && !!schema.default)) {
// Use the default value if it exists
useEffect(() => {
handleChange(path, schema.default || schema.const)
}, [path, schema.default, schema.const, handleChange])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do we feel about setting the default value in teh useState

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the useState not too important here cos the component is not rendered but I realised this also will cause changes even if the original data is defined. Fixed in 5717403.

@dcshzj dcshzj merged commit 7c7c01c into main Jul 24, 2024
16 checks passed
@dcshzj dcshzj deleted the fix/duplicate-dropdown-renderer branch July 24, 2024 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants